Skip to content

Factorize PackageDownloadRunner.TryGetRepositoriesForPackage method and add test#6953

Merged
Nigusu-Allehu merged 10 commits intodevfrom
dev-nyenework-factorize
Nov 25, 2025
Merged

Factorize PackageDownloadRunner.TryGetRepositoriesForPackage method and add test#6953
Nigusu-Allehu merged 10 commits intodevfrom
dev-nyenework-factorize

Conversation

@Nigusu-Allehu
Copy link
Copy Markdown
Member

@Nigusu-Allehu Nigusu-Allehu commented Nov 18, 2025 •
edited
Loading

Bug

Fixes: https://github.com/NuGet/NuGet.Client/pull/6885/files#r2487713673
Fixes: #6885 (comment)

Description

This PR Factorizes the PackageDownloadRunner.TryGetRepositoriesForPackage method, to improve readability. In addition, it adds one more test scenario for the package download command: " no --source, mapping -> A&B, package in both A and B. Latest version from A installed". In addition, it also addresses a bug where http source validation was not being done for package sources when source mapping is enabled and the package is not mapped to any of the sources.

PR Checklist

  • Meaningful title, helpful description and a linked NuGet/Home issue
  • Added tests
  • Link to an issue or pull request to update docs if this PR changes settings, environment variables, new feature, etc.

@Nigusu-Allehu Nigusu-Allehu self-assigned this Nov 18, 2025
@Nigusu-Allehu Nigusu-Allehu marked this pull request as ready for review November 18, 2025 21:10
@Nigusu-Allehu Nigusu-Allehu requested a review from a team as a code owner November 18, 2025 21:10
aortiz-msft
aortiz-msft reviewed Nov 18, 2025
View reviewed changes
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs
Comment thread src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Package/Download/PackageDownloadRunner.cs Outdated
@Nigusu-Allehu Nigusu-Allehu force-pushed the dev-nyenework-factorize branch from 14ae511 to fdc157e Compare November 19, 2025 01:10
jeffkl
jeffkl previously approved these changes Nov 19, 2025
View reviewed changes
zivkan
zivkan previously approved these changes Nov 19, 2025
View reviewed changes
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs
@aortiz-msft
Copy link
Copy Markdown
Collaborator

aortiz-msft commented Nov 19, 2025

@Nigusu-Allehu - How does this implementation differ now from the Restore code path? Any chance of refactoring into a common function?

cc @nkolev92

@Nigusu-Allehu
Copy link
Copy Markdown
Member Author

Nigusu-Allehu commented Nov 20, 2025

@Nigusu-Allehu - How does this implementation differ now from the Restore code path? Any chance of refactoring into a common function?

cc @nkolev92

Oh yes, a refactoring would definitely be nice. One question I had while looking at this: since both restore's remote walker and this command already share the core mapping logic through PackageSourceMapping.GetConfiguredPackageSources(...), do you think there’s much common code left to factor out?

After that call, each path just converts source names into its own representation (IRemoteDependencyProvider in the remote walker vs SourceRepository here)

Curious what others think. happy to adjust if you see a clean way to unify those parts.

@nkolev92
Copy link
Copy Markdown
Member

nkolev92 commented Nov 20, 2025

After that call, each path just converts source names into its own representation (IRemoteDependencyProvider in the remote walker vs SourceRepository here)

Yeah, unfortunately any refactoring would probably add complexity, since it'd probably need to use Func or something to choose the comparer and that just adds allocations.

nkolev92
nkolev92 reviewed Nov 20, 2025
View reviewed changes
Comment thread src/NuGet.Core/NuGet.CommandLine.XPlat/Commands/Package/Download/PackageDownloadRunner.cs Outdated
@aortiz-msft
Copy link
Copy Markdown
Collaborator

aortiz-msft commented Nov 20, 2025 •
edited
Loading

Any tests from the restore implementation that we can copy over or viceversa?

In reply to: 3554996830

aortiz-msft
aortiz-msft requested changes Nov 20, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🕐

@Nigusu-Allehu Nigusu-Allehu dismissed stale reviews from zivkan and jeffkl via 7bd2e43 November 20, 2025 18:19
@Nigusu-Allehu
Copy link
Copy Markdown
Member Author

Nigusu-Allehu commented Nov 20, 2025

Any tests from the restore implementation that we can copy over or viceversa?

In reply to: 3554996830

Missing in PackageDownloadRunnerTests vs FilterDependencyProvidersForLibraryTests:

  • Case-insensitive matching of source names (Added)

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

  • Package pattern mapped to multiple sources all applicable providers returned.

  • Mapping includes a source name that doesn’t exist in the provider list

nkolev92
nkolev92 previously approved these changes Nov 20, 2025
View reviewed changes
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs Outdated
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs Outdated
Comment thread test/NuGet.Core.FuncTests/NuGet.XPlat.FuncTest/Package/Download/PackageDownloadRunnerTests.cs Outdated
@nkolev92 nkolev92 dismissed their stale review November 20, 2025 19:41

sorry, bad click, I meant to comment.

@Nigusu-Allehu Nigusu-Allehu force-pushed the dev-nyenework-factorize branch from f3999ed to 62c2dfa Compare November 21, 2025 02:01
@aortiz-msft
Copy link
Copy Markdown
Collaborator

aortiz-msft commented Nov 24, 2025

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

  • Package pattern mapped to multiple sources all applicable providers returned.
  • Mapping includes a source name that doesn’t exist in the provider list

I'm not seeing tests added to FilterDependencyProvidersForLibraryTests. Could you please add the appropriate tests there as well as part of this PR?

@aortiz-msft aortiz-msft reopened this Nov 24, 2025
aortiz-msft
aortiz-msft requested changes Nov 24, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments

@Nigusu-Allehu
Copy link
Copy Markdown
Member Author

Nigusu-Allehu commented Nov 24, 2025

Missing in FilterDependencyProvidersForLibraryTests vs PackageDownloadRunnerTests :

  • Package pattern mapped to multiple sources all applicable providers returned.
  • Mapping includes a source name that doesn’t exist in the provider list

I'm not seeing tests added to FilterDependencyProvidersForLibraryTests. Could you please add the appropriate tests there as well as part of this PR?

As discussed, offline, added here instead of a separate follow up pr.

aortiz-msft
aortiz-msft approved these changes Nov 25, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved for now, but refactoring to be done in a separate PR.

@Nigusu-Allehu Nigusu-Allehu merged commit 11ea99e into dev Nov 25, 2025
17 of 18 checks passed
@Nigusu-Allehu Nigusu-Allehu deleted the dev-nyenework-factorize branch November 25, 2025 01:05
This was referenced Dec 5, 2025
Merged
Merged
@dotnet-maestro dotnet-maestro Bot mentioned this pull request Feb 20, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aortiz-msft aortiz-msft aortiz-msft approved these changes

@zivkan zivkan zivkan left review comments

@jeffkl jeffkl jeffkl left review comments

@martinrrm martinrrm Awaiting requested review from martinrrm martinrrm is a code owner automatically assigned from NuGet/nuget-client-pull-request-auto-assignment

@nkolev92 nkolev92 Awaiting requested review from nkolev92

Assignees

@Nigusu-Allehu Nigusu-Allehu

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Nigusu-Allehu @aortiz-msft @nkolev92 @zivkan @jeffkl